Issue-125323: ICE non-ADT in struct pattern when long time constant evaluation is in for loop #138679

Shunpoco · 2025-03-18T23:09:04Z

This PR fixes #125323

Context

According to the issue, the ICE happens since #121206.
In the PR, some error methods were reorganized. For example, has_errors() was renamed to has_errors_exclude_lint_errors(). However, some codes which used the original has_errors() were not switched to has_errors_exclude_lint_errors(). I finally found that report_error() in writeback.rs causes this ICE. Currently the method uses tainted_by_errors() to get guar (ErrorGuaranteed), but originally it used dcx().has_errors() but it wasn't changed to has_errors_exclude_lint_errors() when changes in #121206 were merged. I don't think I fully understand how an error is propagated, but I suppose that the error from long time constant evaluation is unexpectedly propagated other parts (in this ICE, for loop), then cause the non-ADT in struct pattern ICE.

Change

Fix report_error() in writeback.rs: use dcx().has_errors_exclude_lint_errors() instead of tainted_by_errors() to prevent error propagation from constant evaluation.
Add test for the ICE
Modify some tests to align the change: Due to this fix, E0282 error happens (or not happen anymore) in some tests.

NOTE

The 4th commit aims to revert the fix in #123516 because I confirmed that the ICE solved by the PR doesn't happen if I modify report_error(). I think the root cause of that ICE is the same as #125323 . But I can discard this commit since we can fix #125323 without it.

rustbot · 2025-03-18T23:09:08Z

r? @estebank

rustbot has assigned @estebank.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2025-03-19T00:51:07Z

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

compiler-errors · 2025-03-19T01:18:20Z

I don't believe this is the right solution, because using tainted_by_errors in Resolver::report_error is intentional to make sure we don't have non-local dependence on error reporting.

The real root cause of this issue is this:

rust/compiler/rustc_interface/src/passes.rs

Lines 991 to 995 in 75530e9

    
           // But we exclude lint errors from this, because lint errors are typically 
        
           // less serious and we're more likely to want to continue (#87337). 
        
           if let Some(guar) = sess.dcx().has_errors_excluding_lint_errors() { 
        
               guar.raise_fatal(); 
        
           }

...which causes us to enter check_mod_deathness if the only error that was reported was through the const eval lint. I think the code in compiler/rustc_passes/src/dead.rs should handle errors more gracefully, and not walk into modules if the body typeck results are tainted. But I'll pass this onto oli who can confirm my suspicions and guide this PR along, since they've been responsible for some of this error tainting and cleanup work.

r? oli-obk

compiler-errors · 2025-03-19T01:18:44Z

Please ensure that after this PR is fully reworked that it's squashed into a single commit.

oli-obk · 2025-03-19T11:58:43Z

Yea errs' analysis is spot on. Skipping the visit_body call if the typeck results are tainted in visit_nested_body would avoid any such ICEs. This would have the fun side effect of causing everything with errors to be marked as dead though, so we likely have to record that we didn't do the analysis properly. The easiest way to do that would be to make the Visitor::Result be an actual Result<(), ErrorGuaranteed> and recording that in the query result.

Shunpoco · 2025-03-21T19:42:04Z

Thanks both!

@oli-obk
I fixed my code in cd7a8f5.
There are two panics in the issue. Skipping visit_body solves the panic in dead.rs, but I also need to add early return in check_pat in builtin.rs.

The easiest way to do that would be to make the Visitor::Result be an actual Result<(), ErrorGuaranteed> and recording that in the query result.

Are you talking about ResultsVisitor in compiler/rustc_mir_dataflow/src/framework/visitor.rs? So far I don't understand how to record the error into the query result. It would be great if you give me a hint which code I should looking for

meithecatte · 2025-03-28T00:54:01Z

Does this fix #138361 as well? Seems like a very closely related issue, at the very least. Ideally we'd make sure that the approach taken will take care of both of these.

Shunpoco · 2025-04-03T21:54:29Z

@meithecatte
Sorry for the late reply!

I think the root cause is the same. I tried your issue's failure case with 3cf7d22, and the ICE didn't happen, although as we discussed above I can't use this commit as the solution.
But I think a solution of your issue may be similar with mine. I will check your issue as well.

@oli-obk @compiler-errors
How does the compiler make tainted_by_error? Is it possible to separate this function into two (tainted_by_error, and something like tainted_by_error_exclude_lint_error)?

oli-obk · 2025-04-28T09:50:09Z

Is it possible to separate this function into two (tainted_by_error, and something like tainted_by_error_exclude_lint_error)?

I tried this before, it is very complex and fragile and comes with a runtime cost. So that's not a solution that can be applied here.

So far I don't understand how to record the error into the query result. It would be great if you give me a hint which code I should looking for

you can change the query result in the query declaration to Result<(), ErrorGuaranteed>. You'll then need to adjust the query provider function's signature, too. I haven't checked what else needs doing, but the compiler should mostly guide you if you also change the visitor and start bubbling the error out all the way to the query provider

Shunpoco · 2025-08-31T11:55:10Z

@oli-obk I spent too much time, but finally I could have some progresses.

I modify the signature of live_symbols_and_ignored_derived_traits. check_mod_deathness calls that query and return earlier if it returns error.
- Note that I don't modify signatures of check_mod_deathness and analysis because those queries are called with ensure_ok(), so returned values do not matter I guess
To make the query return an error, I add Self::Result in MarkSymbolVisitor.
- Signatures of functions are also changed to return Result
- Currently I'm using ControlFlow as Result, but I have no confident if I can use it, or I should define other Result in compiler/rustc_ast_ir/src/visit.rs.
To make sure my new change prevents everything goes to dead, I add new lines in a test code. Without my new change, the test fails with the unused warning

Shunpoco · 2025-08-31T11:56:18Z

@rustbot review

compiler-errors · 2025-09-01T15:55:23Z

Unrelated to review, but please make sure to clean up the commit history and make sure this approach is explained clearly why is it both necessary and sufficient for fixing the ICE.

bors · 2025-09-13T17:46:56Z

☔ The latest upstream changes (presumably #145186) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot · 2025-10-17T17:36:33Z

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

Shunpoco · 2025-10-17T21:38:28Z

@rustbot review

oli-obk · 2025-10-18T11:33:21Z

@bors try @rust-timer queue

Will merge after perf is clean (should be, but wanna make sure rolling this PR up is fine)

Issue-125323: ICE non-ADT in struct pattern when long time constant evaluation is in for loop

rust-bors · 2025-10-18T13:45:36Z

☀️ Try build successful (CI)
Build commit: f20a1c6 (f20a1c68498cd01e010e928c01092746578ae76c, parent: ab1d2444533d829e2d5cff6634cd3c70de6d7103)

rust-timer · 2025-10-18T14:56:17Z

Finished benchmarking commit (f20a1c6): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 1.9%, secondary 2.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.9%	[1.6%, 2.3%]	2
Regressions ❌ (secondary)	2.4%	[2.4%, 2.4%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.9%	[1.6%, 2.3%]	2

Cycles

Results (primary 2.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.1%	[2.1%, 2.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.1%	[2.1%, 2.1%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 473.727s -> 474.422s (0.15%)
Artifact size: 390.45 MiB -> 390.44 MiB (-0.00%)

oli-obk · 2025-10-18T15:07:00Z

@bors r+ rollup

bors · 2025-10-18T15:07:02Z

📌 Commit 845ff73 has been approved by oli-obk

It is now in the queue for this repository.

Issue-125323: ICE non-ADT in struct pattern when long time constant evaluation is in for loop This PR fixes rust-lang#125323 ## Context According to the issue, the ICE happens since rust-lang#121206. In the PR, some error methods were reorganized. For example, has_errors() was renamed to has_errors_exclude_lint_errors(). However, some codes which used the original has_errors() were not switched to has_errors_exclude_lint_errors(). I finally found that report_error() in writeback.rs causes this ICE. Currently the method uses tainted_by_errors() to get guar (ErrorGuaranteed), but originally it used dcx().has_errors() but it wasn't changed to has_errors_exclude_lint_errors() when changes in rust-lang#121206 were merged. I don't think I fully understand how an error is propagated, but I suppose that the error from long time constant evaluation is unexpectedly propagated other parts (in this ICE, for loop), then cause the non-ADT in struct pattern ICE. ## Change - Fix report_error() in writeback.rs: use dcx().has_errors_exclude_lint_errors() instead of tainted_by_errors() to prevent error propagation from constant evaluation. - Add test for the ICE - Modify some tests to align the change: Due to this fix, E0282 error happens (or not happen anymore) in some tests. ## NOTE The 4th commit aims to revert the fix in rust-lang#123516 because I confirmed that the ICE solved by the PR doesn't happen if I modify report_error(). I think the root cause of that ICE is the same as rust-lang#125323 . But I can discard this commit since we can fix rust-lang#125323 without it.

Rollup of 8 pull requests Successful merges: - #138679 (Issue-125323: ICE non-ADT in struct pattern when long time constant evaluation is in for loop) - #146167 (Deny-by-default never type lints) - #146490 (Rehome 26 `tests/ui/issues/` tests to other subdirectories under `tests/ui/` [#5 of Batch #2]) - #147438 (Rename "non-inline module" to "file module" in proc macro diagnostics) - #147724 (Fix ICE in pattern matching with generic const array length errors) - #147813 (Warn on unused_attributes in uitests ) - #147816 (Do not error out for `download-rustc` if LTO is configured) - #147845 (Add regression test for 134355) r? `@ghost` `@rustbot` modify labels: rollup

Issue-125323: ICE non-ADT in struct pattern when long time constant evaluation is in for loop This PR fixes rust-lang#125323 ## Context According to the issue, the ICE happens since rust-lang#121206. In the PR, some error methods were reorganized. For example, has_errors() was renamed to has_errors_exclude_lint_errors(). However, some codes which used the original has_errors() were not switched to has_errors_exclude_lint_errors(). I finally found that report_error() in writeback.rs causes this ICE. Currently the method uses tainted_by_errors() to get guar (ErrorGuaranteed), but originally it used dcx().has_errors() but it wasn't changed to has_errors_exclude_lint_errors() when changes in rust-lang#121206 were merged. I don't think I fully understand how an error is propagated, but I suppose that the error from long time constant evaluation is unexpectedly propagated other parts (in this ICE, for loop), then cause the non-ADT in struct pattern ICE. ## Change - Fix report_error() in writeback.rs: use dcx().has_errors_exclude_lint_errors() instead of tainted_by_errors() to prevent error propagation from constant evaluation. - Add test for the ICE - Modify some tests to align the change: Due to this fix, E0282 error happens (or not happen anymore) in some tests. ## NOTE The 4th commit aims to revert the fix in rust-lang#123516 because I confirmed that the ICE solved by the PR doesn't happen if I modify report_error(). I think the root cause of that ICE is the same as rust-lang#125323 . But I can discard this commit since we can fix rust-lang#125323 without it.

Rollup of 7 pull requests Successful merges: - #138679 (Issue-125323: ICE non-ADT in struct pattern when long time constant evaluation is in for loop) - #146490 (Rehome 26 `tests/ui/issues/` tests to other subdirectories under `tests/ui/` [#5 of Batch #2]) - #147438 (Rename "non-inline module" to "file module" in proc macro diagnostics) - #147724 (Fix ICE in pattern matching with generic const array length errors) - #147813 (Warn on unused_attributes in uitests ) - #147816 (Do not error out for `download-rustc` if LTO is configured) - #147845 (Add regression test for 134355) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of #138679 - Shunpoco:issue-125323, r=oli-obk Issue-125323: ICE non-ADT in struct pattern when long time constant evaluation is in for loop This PR fixes #125323 ## Context According to the issue, the ICE happens since #121206. In the PR, some error methods were reorganized. For example, has_errors() was renamed to has_errors_exclude_lint_errors(). However, some codes which used the original has_errors() were not switched to has_errors_exclude_lint_errors(). I finally found that report_error() in writeback.rs causes this ICE. Currently the method uses tainted_by_errors() to get guar (ErrorGuaranteed), but originally it used dcx().has_errors() but it wasn't changed to has_errors_exclude_lint_errors() when changes in #121206 were merged. I don't think I fully understand how an error is propagated, but I suppose that the error from long time constant evaluation is unexpectedly propagated other parts (in this ICE, for loop), then cause the non-ADT in struct pattern ICE. ## Change - Fix report_error() in writeback.rs: use dcx().has_errors_exclude_lint_errors() instead of tainted_by_errors() to prevent error propagation from constant evaluation. - Add test for the ICE - Modify some tests to align the change: Due to this fix, E0282 error happens (or not happen anymore) in some tests. ## NOTE The 4th commit aims to revert the fix in #123516 because I confirmed that the ICE solved by the PR doesn't happen if I modify report_error(). I think the root cause of that ICE is the same as #125323 . But I can discard this commit since we can fix #125323 without it.

rustbot assigned estebank Mar 18, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 18, 2025